This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Use same Task for Task.CompletedTask and ATMB.Completed #24404
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephentoub
reviewed
May 6, 2019
src/System.Private.CoreLib/shared/System/Threading/Tasks/Task.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
May 6, 2019
| @@ -270,7 +263,7 @@ public Task Task | |||
| /// </summary> | |||
| /// <exception cref="System.InvalidOperationException">The builder is not initialized.</exception> | |||
| /// <exception cref="System.InvalidOperationException">The task has already completed.</exception> | |||
| public void SetResult() => m_builder.SetResult(s_cachedCompleted); // Using s_cachedCompleted is faster than using s_defaultResultTask. | |||
| public void SetResult() => m_builder.SetResult(Task.s_cachedCompleted); // Using s_cachedCompleted is faster than using s_defaultResultTask via generic. | |||
There was a problem hiding this comment.
Have you confirmed the asm for this is just as good?
There was a problem hiding this comment.
Yep, ends up identical
[12 IL=0141 TR=000050 060036BE] [profitable inline] AsyncTaskMethodBuilder:SetResult():this
[13 IL=0011 TR=000240 060036D0] [profitable inline] AsyncTaskMethodBuilder`1:SetResult(ref):this
[0 IL=0026 TR=000260 060036CE] [FAILED: too many basic blocks] AsyncTaskMethodBuilder`1:SetExistingTaskResult(struct):this
...
; Total bytes of code 339, prolog size 29 for method <Main>d__0:MoveNext():this
Its going via a generic rather than non-generic that causes the speed bump
There was a problem hiding this comment.
Yep, ends up identical
More or less
- BAA4040000 mov edx, 0x4A4
+ BA84020000 mov edx, 644
E89B83B25F call CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
- 48BA80190AD3E5010000 mov rdx, 0x1E5D30A1980
+ 48BA381647D6FC010000 mov rdx, 0x1FCD6471638
488B12 mov rdx, gword ptr [rdx]
|
CI looks quite broken |
jkotas
reviewed
May 6, 2019
src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncMethodBuilder.cs
Outdated
Show resolved
Hide resolved
|
Code gen ends up identical in usage (other than referring to same object) and a change in .cctors |
|
Follow up that takes this a bit further #24431 |
|
Closing in preference of #24431 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Keep it hotter in cache rather than alternating between the two for manually and compiler generated async methods
/cc @stephentoub